Change the desugaring of `assert!` for better error output #122661

estebank · 2024-03-17T21:19:33Z

In the desugaring of assert!, we now expand to a match expression instead of if !cond {..}.

The span of incorrect conditions will point only at the expression, and not the whole assert! invocation.

error[E0308]: mismatched types
  --> $DIR/issue-14091.rs:2:13
   |
LL |     assert!(1,1);
   |             ^ expected `bool`, found integer

We no longer mention the expression needing to implement the Not trait.

error[E0308]: mismatched types
  --> $DIR/issue-14091-2.rs:15:13
   |
LL |     assert!(x, x);
   |             ^ expected `bool`, found `BytePos`

Now assert!(val) desugars to:

match val {
    true => {},
    _ => $crate::panic::panic_2021!(),
}

Fix #122159.

rustbot · 2024-03-17T21:19:41Z

r? @pnkfelix

rustbot has assigned @pnkfelix.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-03-17T21:19:43Z

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

The Miri subtree was changed

cc @rust-lang/miri

estebank · 2024-03-17T21:20:59Z

src/tools/miri/tests/pass/binops.rs

-    assert!((() <= ()));
-    assert!((!(() > ())));
-    assert!((() >= ()));
+    assert!(!(() != ()));


All of these are because now the unnecessary parentheses lint (correctly) triggers on assert!((expr)).

estebank · 2024-03-17T21:22:05Z

tests/ui/issues/issue-14091-2.stderr

-error[E0600]: cannot apply unary operator `!` to type `BytePos`
-  --> $DIR/issue-14091-2.rs:15:5
+error[E0308]: mismatched types
+  --> $DIR/issue-14091-2.rs:15:13
   |
 LL |     assert!(x, x);
-   |     ^^^^^^^^^^^^^ cannot apply unary operator `!`
-   |
-note: an implementation of `Not` might be missing for `BytePos`
-  --> $DIR/issue-14091-2.rs:6:1
-   |
-LL | pub struct BytePos(pub u32);
-   | ^^^^^^^^^^^^^^^^^^ must implement `Not`
-note: the trait `Not` must be implemented
-  --> $SRC_DIR/core/src/ops/bit.rs:LL:COL
-   = note: this error originates in the macro `assert` (in Nightly builds, run with -Z macro-backtrace for more info)


This is the best example of how this is an improvement.

estebank · 2024-03-17T22:41:11Z

Sigh, clippy shows at least one test where a suggestion causes there to be a condition that isn't a bool but rather a type where Not returns a bool. We can easily continue supporting that by making the desugaring !!cond, but would rather not do that. This happens when it suggest changing assert_eq!(non_bool, true), ending up as assert!(non_bool), which doesn't work with this change .

pnkfelix · 2024-03-18T15:18:59Z

Isn't this technically a breaking change for e.g. (playground):

struct Booly(i32);

impl std::ops::Not for Booly {
    type Output = bool;
    fn not(self) -> Self::Output {
        self.0 == 0
    }
}

fn main() {
    assert!(Booly(1), "booly booly!")
}

pnkfelix · 2024-03-18T15:25:11Z

At the very least, we might need to tie such a change to an edition.

I am not certain whether this decision would be a T-lang matter or a T-libs-api one. I'll nominate for T-lang for now.

(Namely: The question is whether we can start enforcing a rule that the first expression to assert! must be of bool type, which is how the macro is documented, but its current behavior is a little bit more general, as demonstrated in my prior comment)

@rustbot label +I-lang-nominated

src/tools/rust-analyzer/crates/parser/test_data/parser/ok/0035_weird_exprs.rs

estebank · 2024-03-18T15:33:43Z

@pnkfelix we can keep the current (undocumented) behavior by making the desugaring be

{
    let x: bool = !!condition;
    x
}

instead of what this PR does:

{
    let x: bool = condition;
    x
}

I believe that would still cause errors to complain about Not not being implemented, instead of the more straightforward type error, albeit with a better span. I don't particularly like the idea of keeping the current emergent behavior if there aren't people exploiting it in crates.io.

Edit: an option would be to have an internal marker trait:

use std::ops::Not;
trait CanAssert {}
impl<T: Not<Output = bool>> CanAssert for T {}

fn main() {
    let _ = Box::new(true) as Box<dyn CanAssert>;
    let _ = Box::new(42) as Box<dyn CanAssert>;
}

error[E0271]: type mismatch resolving `<i32 as Not>::Output == bool`
 --> src/main.rs:7:13
  |
7 |     let _ = Box::new(42) as Box<dyn CanAssert>;
  |             ^^^^^^^^^^^^ expected `bool`, found `i32`
  |
note: required for `i32` to implement `CanAssert`
 --> src/main.rs:3:29
  |
3 | impl<T: Not<Output = bool>> CanAssert for T {}
  |             -------------   ^^^^^^^^^     ^
  |             |
  |             unsatisfied trait bound introduced here
  = note: required for the cast from `Box<i32>` to `Box<dyn CanAssert>`

pnkfelix · 2024-03-18T17:00:27Z

@estebank what about making the expansion edition-dependent? Is there precedent for that?

Then, editions >= 2024 would expand to what you have proposed in the code of this PR, and editions < 2024 could expand to the !!condition variant form that you have discussed in the comments?

pnkfelix · 2024-03-18T17:53:23Z

what about making the expansion edition-dependent? Is there precedent for that?

(to answer my own question, panic! is one obvious precedent here. So it seems like making it edition-dependent would be one acceptable path; no opinion yet as to which is best...)

rustbot · 2024-03-19T00:05:12Z

Some changes occurred in coverage tests.

cc @Zalathar

estebank · 2024-03-19T00:05:14Z

I tried the marker trait approach for <=2021, and it kind of worked, but the diagnostics were actually worse than just doing { let x: bool = !!$expr; x }, which accounts for pretty much everything we currently support, but with better spans and better errors (if typeof($expr) implements <Not<Output = NotBool>, we now produce an appropriate E0308 type error).

compiler-errors · 2024-03-19T16:40:43Z

Since I don't think it's been acknowledged above, for the record, this breaks the following code:

fn hello(x: &bool) {
  assert!(x);
}

Because &bool: Not<Output = bool>.

estebank · 2024-03-19T17:27:56Z

@compiler-errors that is indeed the case for 2024 onwards, not for previous editions.

pnkfelix · 2024-03-20T14:45:31Z

@compiler-errors that is indeed the case for 2024 onwards, not for previous editions.

I think the critical point is whether an edition-dependent expansion is worth breaking that case (of assert!(x) where x: &bool), or if we should do a non-breaking non-edition-dependent expansion using the let x: bool = !!$expr trick across the board...

Update: I don't know whether it is worth going through this exercise explicitly, but there is a design space here. E.g. one set of options is:

(stable Rust behavior): in all editions, support arbitrary impl Not<Output=bool> for first parameter to assert!;
in edition >= 2024, support just Deref<Target=bool> for first parameter to assert! (e.g. by expanding to let x: &bool = &$expr;), or
(this PR): in edition >= 2024, support just bool for first parameter to assert!.

(And then there's variations thereof about how to handle editions < 2024, but that's a separate debate IMO.)

pnkfelix · 2024-03-22T16:12:22Z

(this is waiting for a decision from T-lang and/or T-libs regarding what interface we want to commit to for assert!)

@rustbot label: +S-waiting-on-team -S-waiting-on-review

rfcbot · 2025-05-16T14:59:40Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

compiler/rustc_builtin_macros/src/assert.rs

estebank · 2025-05-21T09:45:13Z

src/tools/clippy/tests/ui/incompatible_msrv.stderr

+error: sub-expression diverges
+  --> tests/ui/incompatible_msrv.rs:59:13
+   |
+LL |     assert!(core::panicking::panic("out of luck"));
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::diverging-sub-expression` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::diverging_sub_expression)]`


@rust-lang/clippy would this be considered a regression? The lint triggers here now because of the change to the expansion of assert!.

It looks like a lint ordering issue (lint order is not defined). I would add #[allow(clippy::diverging_sub_expression)] (as this is not the lint this test wants to trigger) and check that the original diagnostic still stands.

Although, having cross-lint testing could really help ensure not foreseen interaction between lints. Having some lints in unrelated files is another way of testing their behaviour against unexpected tokens. (Like what lintcheck does)

I'd keep it, but I'm not sure if the team has an consistent opinion on this. Our test suite is kinda a mixed bag of allows and not allows.

I'd rather allow unrelated lints.

I agree that cross-checking lints would be nice. But doing that in some tests won't give us any guarantees, but just increases the number of files that need to be updated for changes.

From my experience on rustc, the number of errors being affected by changes isn't that big of a deal and has helped caught issues during development, but changed this test to avoid it, leaving it to the team to decide whether to add cross-lint tests at your leisure.

estebank · 2025-05-22T09:45:35Z

Fun, can't repro the miri failure locally on my cloud dev desktop :-/

bors · 2025-07-13T18:41:02Z

☔ The latest upstream changes (presumably #143888) made this pull request unmergeable. Please resolve the merge conflicts.

In the desugaring of `assert!`, we now expand to a `match` expression instead of `if !cond {..}`. The span of incorrect conditions will point only at the expression, and not the whole `assert!` invocation. ``` error[E0308]: mismatched types --> $DIR/issue-14091.rs:2:13 | LL | assert!(1,1); | ^ expected `bool`, found integer ``` We no longer mention the expression needing to implement the `Not` trait. ``` error[E0308]: mismatched types --> $DIR/issue-14091-2.rs:15:13 | LL | assert!(x, x); | ^ expected `bool`, found `BytePos` ``` `assert!(val)` now desugars to: ```rust match val { true => {}, _ => $crate::panic::panic_2021!(), } ``` Fix rust-lang#122159. We make some minor changes to some diagnostics to avoid span overlap on type mismatch or inverted "expected"/"found" on type errors. We remove some unnecessary parens from core, alloc and miri.

rustbot · 2025-07-17T19:35:01Z

⚠️ Warning ⚠️

There are issue links (such as #123) in the commit messages of the following commits.
Please move them to the PR description, to avoid spamming the issues with references to the commit, and so this bot can automatically canonicalize them to avoid issues with subtree.
- b3ffb52

estebank · 2025-07-18T18:14:27Z

As per #122661 (comment) and #122661 (comment), I believe this can be merged. The error I couldn't reproduce no longer seems to be triggering.

Running crater just in case.

@craterbot run mode=check-only

craterbot · 2025-07-18T18:14:30Z

🚨 Error: missing start toolchain

🆘 If you have any trouble with Crater please ask in t-infra on Zulip
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

estebank · 2025-07-18T18:15:13Z

@bors try

Change the desugaring of `assert!` for better error output In the desugaring of `assert!`, we now expand to a `match` expression instead of `if !cond {..}`. The span of incorrect conditions will point only at the expression, and not the whole `assert!` invocation. ``` error[E0308]: mismatched types --> $DIR/issue-14091.rs:2:13 | LL | assert!(1,1); | ^ expected `bool`, found integer ``` We no longer mention the expression needing to implement the `Not` trait. ``` error[E0308]: mismatched types --> $DIR/issue-14091-2.rs:15:13 | LL | assert!(x, x); | ^ expected `bool`, found `BytePos` ``` Now `assert!(val)` desugars to: ```rust match val { true => {}, _ => $crate::panic::panic_2021!(), } ``` Fix #122159.

bors · 2025-07-18T18:16:25Z

⌛ Trying commit b3ffb52 with merge a2a6e87...

bors · 2025-07-18T20:30:11Z

☀️ Try build successful - checks-actions
Build commit: a2a6e87 (a2a6e871b05c7b8ecc9a3c8a20db1f39e995e399)

estebank · 2025-07-20T02:51:56Z

@craterbot check

craterbot · 2025-07-20T02:52:16Z

👌 Experiment pr-122661 created and queued.
🤖 Automatically detected try build a2a6e87
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

rustbot assigned pnkfelix Mar 17, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 17, 2024

estebank commented Mar 17, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

estebank force-pushed the assert-macro-span branch from ead1593 to eb411c1 Compare March 17, 2024 21:27

This comment has been minimized.

Sign in to view

This comment was marked as resolved.

Sign in to view

This comment has been minimized.

Sign in to view

pnkfelix mentioned this pull request Mar 18, 2024

inconsistent and confusing error message about first argument of assert! #122159

Open

rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Mar 18, 2024

Veykril reviewed Mar 18, 2024

View reviewed changes

src/tools/rust-analyzer/crates/parser/test_data/parser/ok/0035_weird_exprs.rs Outdated Show resolved Hide resolved

estebank force-pushed the assert-macro-span branch from c8185ea to 07a5b21 Compare March 18, 2024 23:59

This comment has been minimized.

Sign in to view

rfcbot added the to-announce Announce this issue on triage meeting label May 16, 2025

rustbot mentioned this pull request May 16, 2025

Tracking issue for release notes of #122661: Change the desugaring of assert! for better error output #141089

Open

teor2345 reviewed May 18, 2025

View reviewed changes

compiler/rustc_builtin_macros/src/assert.rs Outdated Show resolved Hide resolved

estebank force-pushed the assert-macro-span branch 2 times, most recently from 3fca21e to 6063e7f Compare May 21, 2025 09:43

estebank commented May 21, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

estebank force-pushed the assert-macro-span branch from 6063e7f to 1559a5b Compare May 22, 2025 08:22

This comment has been minimized.

Sign in to view

estebank force-pushed the assert-macro-span branch from 1559a5b to 51f8144 Compare May 22, 2025 10:06

This comment has been minimized.

Sign in to view

apiraino removed the to-announce Announce this issue on triage meeting label Jun 5, 2025

estebank force-pushed the assert-macro-span branch from 51f8144 to 1662721 Compare July 8, 2025 18:21

estebank force-pushed the assert-macro-span branch from 1662721 to b3ffb52 Compare July 17, 2025 19:34

rustbot added the T-clippy Relevant to the Clippy team. label Jul 17, 2025

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 20, 2025

Change the desugaring of assert! for better error output #122661

Are you sure you want to change the base?

Change the desugaring of assert! for better error output #122661

Conversation

estebank commented Mar 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Mar 17, 2024

Uh oh!

rustbot commented Mar 17, 2024

Uh oh!

estebank Mar 17, 2024

Choose a reason for hiding this comment

Uh oh!

estebank Mar 17, 2024

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

estebank commented Mar 17, 2024

Uh oh!

This comment was marked as resolved.

This comment has been minimized.

pnkfelix commented Mar 18, 2024

Uh oh!

pnkfelix commented Mar 18, 2024

Uh oh!

Uh oh!

estebank commented Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pnkfelix commented Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pnkfelix commented Mar 18, 2024

Uh oh!

rustbot commented Mar 19, 2024

Uh oh!

estebank commented Mar 19, 2024

Uh oh!

This comment has been minimized.

compiler-errors commented Mar 19, 2024

Uh oh!

estebank commented Mar 19, 2024

Uh oh!

pnkfelix commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pnkfelix commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rfcbot commented May 16, 2025

Uh oh!

Uh oh!

estebank May 21, 2025

Choose a reason for hiding this comment

Uh oh!

samueltardieu May 21, 2025

Choose a reason for hiding this comment

Uh oh!

blyxyas May 21, 2025

Choose a reason for hiding this comment

Uh oh!

flip1995 May 21, 2025

Choose a reason for hiding this comment

Uh oh!

estebank May 22, 2025

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

estebank commented May 22, 2025

Uh oh!

This comment has been minimized.

bors commented Jul 13, 2025

Uh oh!

rustbot commented Jul 17, 2025

Uh oh!

estebank commented Jul 18, 2025

Uh oh!

Change the desugaring of `assert!` for better error output #122661

Change the desugaring of `assert!` for better error output #122661

estebank commented Mar 17, 2024 •

edited

Loading

estebank commented Mar 18, 2024 •

edited

Loading

pnkfelix commented Mar 18, 2024 •

edited

Loading

pnkfelix commented Mar 20, 2024 •

edited

Loading

pnkfelix commented Mar 22, 2024 •

edited

Loading